Skip to content

Conversation

bartholdbos
Copy link

No description provided.

@bartholdbos bartholdbos force-pushed the softdeleteable-deleted-value branch from 4e6a1f2 to 6264492 Compare June 18, 2025 13:35
@bartholdbos
Copy link
Author

@phansys We have added some tests like the existing ones, does this meet the requirements?

@mbabker
Copy link
Contributor

mbabker commented Jul 3, 2025

You mind describing the use case here? Also, just on a glance over this PR it seems like a lot more changes than this would be needed considering the extension is designed around using nullable date columns for the deleted value, so widening it to support non-date values in place of null feels like it would need more changes.

You've also only updated the ORM's filter class, the ODM's should be updated as well to keep both implementations working the same way.

As for the tests, new test fixtures should be added which cover the new configuration option and not just adding it to the existing tests.

@RubenKluft
Copy link

The use case is as follows:
Mysql: "UNIQUE index permits multiple NULL values for columns that can contain NULL."
https://dev.mysql.com/doc/refman/8.4/en/create-index.html
(which any soft delete column currently must be)
If there is a case where one would like to only allow one not-softdeleted value, this is currently impossible with softdelete.
Allowing a different value than null to be set to be seen as not-deleted for the softdelete column would permit allowing a not-deleted softdeletable entries to be unique.

@RubenKluft
Copy link

@mbabker Do you consider new test satisfactory?
Otherwise I'd like some guidance on what exactly you'd like to see tested and how.

@RubenKluft
Copy link

@phansys We'd love to get this merged, any help you can provide to help us to reach that point would be much appreciated.

@jaaplallie
Copy link

@phansys @mbabker Hello, we assume you guys have a backlog with items to work through, but could you give an indication on how / when we can proceed? Thank you in advance :)

@mbabker
Copy link
Contributor

mbabker commented Aug 19, 2025

I don't have merge rights so I can't do anything with this. But based on the current state of the PR:

  • I'd revert the changes in the ‎tests/Gedmo/Mapping/Driver/Xml/Gedmo.Tests.Mapping.Fixture.Xml.SoftDeleteable.dcm.xml, ‎tests/Gedmo/Mapping/Driver/Yaml/Gedmo.Tests.Mapping.Fixture.Yaml.SoftDeleteable.dcm.yml, and ‎tests/Gedmo/Mapping/Fixture/SoftDeleteable.php test fixtures so those continue to test the default behaviors (which would also imply reverting the changes in ‎SoftDeleteableMappingTest)
  • For mapping tests, we'd want new fixtures with the new config option to validate the mapping part of things (a lot of the fixtures are just copy/paste classes and adapted to different config scenarios)
  • The added test case in SoftDeleteableEntityTest is good, I'd add a similar case for the SoftDeleteableDocumentTest to cover the MongoDB part of this (but do double check the added fixtures to make sure the annotations and attributes are consistent, the UserNonDeletedValue fixture has different configs so the test builds using annotations are going to have different behaviors from the builds using attributes)

My one outstanding concern is that the current state of the fixtures (especially the #[Gedmo\SoftDeleteable(fieldName: 'deletedAt', nonDeletedColumnValue: '0')] attribute) implies that this should support non-datetime values. If that's intended, then there should be at least one test fixture and case in the document and entity tests to ensure this is working. If it's intended for this to only support datetime-ish values or null, then there should probably be a check for this in the mapping drivers so users don't misconfigure things and file bug reports because things aren't working with bad configs in place.

@RubenKluft
Copy link

RubenKluft commented Aug 27, 2025

@mbabker Thank you for the clear feedback, I have made the requested changes.

It is indeed intended to work with datetime-ish or null values, and I have updated everywhere 0 was used to be DateTime instead.
There is already Validator::validateField in the mappers that validates that only datetime-ish values are used.

@phansys It would be greatly appreciated if this could be merged.

@bartholdbos
Copy link
Author

@phansys Could you maybe take a look at this? Thanks in advance

…d-value

# Conflicts:
#	doc/softdeleteable.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants